Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce sidebar bookmarks page #11897

Merged
merged 1 commit into from
Jan 31, 2022
Merged

Introduce sidebar bookmarks page #11897

merged 1 commit into from
Jan 31, 2022

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jan 19, 2022 •
edited
Loading

Resolves brave/brave-browser#20500

Applied bookmark part of chromium's side panel to sidebar bookmark panel.
sidebar.mojom is used for interacting between sidebar's bookmarks panel with SidebarBookmarksPageHandler.
Most of webui impls in browser/resources/sidebar/bookmarks are copied from upstream
and modified to show bookmark part only. (side panel's page shows read later and bookmark part both.)
Bookmarks page uses custom context menu by asking to native via Sidebar:ShowCustomContextMenu interface.

Resources from Figma:
https://www.figma.com/file/sS3l8tqUFxt54MEiBTalUI/Desktop-toolbar-and-menu?node-id=9%3A5824

macOS light and dark theme.
Screen Shot 2022-01-24 at 9 09 35 PM
Screen Shot 2022-01-24 at 9 09 53 PM

Linux light and dark theme.
Screenshot from 2022-01-24 17-51-19
Screenshot from 2022-01-24 17-50-56

Bookmark item's context menu
Screen Shot 2022-01-25 at 11 22 20 AM

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  1. Launch Brave and enable sidebar feature from brave://flags and re-launch
  2. Open bookmarks item and bookmark items are displayed properly
  3. Test bookmark item's drag and drop and all menus from its context menu

@github-actions github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Jan 19, 2022
@simonhong simonhong force-pushed the sidebar_bookmarks branch 6 times, most recently from 5976611 to 1cc7f90 Compare January 25, 2022 02:15
@simonhong simonhong changed the title WIP: Introduce sidebar bookmarks page Introduce sidebar bookmarks page Jan 25, 2022
@simonhong simonhong marked this pull request as ready for review January 25, 2022 02:30
@simonhong simonhong requested a review from a team as a code owner January 25, 2022 02:30
@github-actions github-actions bot added rebase and removed rebase labels Jan 25, 2022
@simonhong simonhong force-pushed the sidebar_bookmarks branch 2 times, most recently from fc41836 to 5d8838c Compare January 25, 2022 06:51
@simonhong simonhong requested a review from a team as a code owner January 25, 2022 12:01
@bsclifton
Copy link
Member

bsclifton commented Jan 26, 2022 •
edited
Loading

Really curious what you think @petemill for the forking from https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/read_later/side_panel/

It's great that we can make any change we'd like (and also we won't get any unexpected changes on Chromium upgrade). But I would be curious if we did want to "rebase", what that would look like. We for sure have to fork any files which are referencing the BRAVE resource IDs - but the logic like keyboard navigation (ex: bookmark_folder.ts, bookmarks_list.ts) I could see getting upstream bug fixes

bsclifton
bsclifton approved these changes Jan 26, 2022
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks great to me 😄 Left a comment about how macOS is weird with no open folder icon... but this is consistent with existing behavior (macOS doesn't have open folder right now)

Really nice work! 😄 It works great with keyboard shortcuts too (both keyboard navigation and using keyboard shortcuts like shift + click)

@simonhong simonhong requested a review from goodov January 27, 2022 11:31
@simonhong
Copy link
Member Author

Kindly ping :)

goodov
goodov reviewed Jan 28, 2022
View reviewed changes
browser/resources/sidebar/bookmarks/bookmark_folder.html Show resolved Hide resolved
browser/resources/sidebar/bookmarks/bookmark_folder.ts Show resolved Hide resolved
browser/resources/sidebar/bookmarks/bookmark_folder.ts Show resolved Hide resolved
browser/resources/sidebar/bookmarks/bookmarks_api_proxy.ts Show resolved Hide resolved
browser/resources/sidebar/bookmarks/bookmarks_drag_manager.ts Show resolved Hide resolved
browser/ui/views/sidebar/sidebar_container_view.cc Show resolved Hide resolved
browser/ui/views/sidebar/sidebar_container_view.h Outdated Show resolved Hide resolved
browser/ui/webui/sidebar/sidebar_bookmarks_page_handler.cc Show resolved Hide resolved
browser/ui/webui/sidebar/sidebar_bookmarks_page_handler.cc Show resolved Hide resolved
browser/ui/webui/sidebar/sidebar_bookmarks_page_handler.cc Show resolved Hide resolved
@simonhong
Copy link
Member Author

@goodov Thanks for review. All addressed. PTAL again :)

@simonhong simonhong requested a review from goodov January 30, 2022 09:33
goodov
goodov approved these changes Jan 31, 2022
View reviewed changes
Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@simonhong
Introduce sidebar bookmarks page
86f1dc0 
fix brave/brave-browser#20500

Applied bookmark part of chromium's side panel to sidebar bookmark panel.
sidebar.mojom is used for interacting between sidebar's bookmarks panel
with SidebarBookmarksPageHandler.
Most of webui impls in browser/resources/sidebar/bookmarks are copied from upstream
and modified to show bookmark part only.
(side panel's page shows read later and bookmark part both.)
Bookmarks page uses custom context menu by asking to native via
Sidebar:ShowCustomContextMenu interface.
@simonhong simonhong added the CI/skip Do not run CI builds (except noplatform) label Jan 31, 2022
@simonhong
Copy link
Member Author

Set CI/skip label as previous run was all green.
Merged after squash.

@simonhong simonhong merged commit 4f91039 into master Jan 31, 2022
@simonhong simonhong deleted the sidebar_bookmarks branch January 31, 2022 15:16
@simonhong simonhong added this to the 1.37.x - Nightly milestone Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goodov goodov goodov approved these changes

@bsclifton bsclifton bsclifton approved these changes

@nullhook nullhook Awaiting requested review from nullhook

@petemill petemill Awaiting requested review from petemill

Assignees

@simonhong simonhong

Labels
CI/skip Do not run CI builds (except noplatform) potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false
Projects
None yet
Milestone
1.37.x - Release
Development

Successfully merging this pull request may close these issues.

Use side-panel's bookmark part as sidebar's bookmark panel
3 participants
@simonhong @bsclifton @goodov